Having no username set in the RevisionableListener doesn't work because ->setUsername() expects a string even though it is nullable in the database

Having no username set in the RevisionableListener doesn't work because ->setUsername() expects a string even though it is nullable in the database

Thanks, I've updated everything to account for that.

Codecov Report

❌ Patch coverage is 85.10638% with 84 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.95%. Comparing base (eb53dfc) to head (27becd3).

@@            Coverage Diff             @@
##             main    #2825      +/-   ##
==========================================
+ Coverage   78.56%   78.95%   +0.39%     
==========================================
  Files         169      182      +13     
  Lines        8803     9366     +563     
==========================================
+ Hits         6916     7395     +479     
- Misses       1887     1971      +84     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

What's the status of this PR? Anything I can do to help/test? Would like to see it merged soon!

What's the status of this PR? Anything I can do to help/test? Would like to see it merged soon!

I don't think I have anything big to add to this PR at this point. It really just needs testing and making sure the idea is solid (especially as part of the rationale for a new extension versus trying to make a version of loggable that is DBAL 4 friendly is allowing both the old and new data to live side-by-side so an app can either keep that old data by either polyfilling the deprecated DBAL array type or migrating it into the new extension).

I started trying to test/use this in an ODM configuration.

Notables so far:

• RevisionableListener is final - I was previously extending LoggableListener, but perhaps will be able to make the necessary changes in a RevisionInterface implementation instead.
• RevisionInterface "sets" has lots of void returns - In LogEntryInterface these weren't hardcoded, perhaps not the end of the world.
• RevisionInterface has a lot of string that are no longer nullable - Again, probably fine for now.
• Missing StofDoctrineExtensionsBundle integration - Biggest stumpling block so far as event listeners aren't triggering at all.
• No documentation/examples - i.e. doc/revisionable.md and updated doc/annotations.md

So at the moment I've not been able to create/trigger any revisions, I guess there is a way to enable this without StofDoctrineExtensionsBundle, but if you were able to create a dev version with support then I'll be able to test further.

EDIT:

Yes, the nullable change's are causing problems.
e.g.

ERROR: InvalidNullableReturnType - src/Document/Revision.php:82:34 - The declared return type 'string' for
App\Document\Revision::getAction is not nullable, but 'null|string' contains null (see https://psalm.dev/144)
public function getAction(): string

RevisionableListener is final - I was previously extending LoggableListener, but perhaps will be able to make the necessary changes in a RevisionInterface implementation instead.

The loggable listener has to be open because of its prePersistLogEntry hook point; with this implementation, I decided to rely on the events that the Doctrine object managers emit and get rid of that, and as a result, I've landed on a final class.

Maybe it's too soon to go with a hard final listener, but I'm not going to take the "no I'm not going to open it up for inheritance" standpoint if there's a legitimate use case that a hard final blocks. Maybe somewhere down the road, something can be figured out for how interfaces can be built for the listeners (as right now the hook points into Doctrine are all Doctrine event listeners, so the interface is more so specifying the required events to subscribe to and less what a public-ish API looks like).

RevisionInterface "sets" has lots of void returns - In LogEntryInterface these weren't hardcoded, perhaps not the end of the world.

The new extension is more strictly typed given it's being written as new code with the PHP 7.4 minimum in mind, compared to all of the other extensions which were written in PHP 5 times; LogEntryInterface already uses @return void annotations so RevisionInterface solidifies this with native return types.

RevisionInterface has a lot of string that are no longer nullable - Again, probably fine for now.

I wanted to make the revision model a little more strict in terms of having valid state, hence the introduction of the static RevisionInterface::createRevision() constructor and the listener using that over new Revision(); the three fields this effects are the action, version, and logged at timestamp (all of which can be initialized in those constructors), the rest of the fields do remain nullable as the model is designed for everything else to support null values (you don't have to have a user for a revision, you don't have to log the data state for a revision, etc.).

Missing StofDoctrineExtensionsBundle integration - Biggest stumpling block so far as event listeners aren't triggering at all.

That'd have to be done after this PR landed. Trying to shoot a PR off over there would just result in a PR with constantly failing builds, it's easier to handle that update after this change lands.

For manual config in a Symfony app, you can take the loggable listener service in https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/doc/frameworks/symfony.md#extensions-compatible-with-all-managers and change that to the revisionable listener (it'll hook into the same events so it's just changing the service ID and class name)

No documentation/examples - i.e. doc/revisionable.md and updated doc/annotations.md

It'll get done before this merges. I did call out the needed mapping changes in the PR description, but the gist of it is you switch @Gedmo\Loggable to @Gedmo\Revisionable for annotations and attributes, and if you're using the logEntryClass param to use a custom model, change that to revisionClass. The @Gedmo\Versioned annotation/attribute gets reused, with the goal here being to make the config migration as minimal as possible.

If action is no longer nullable does it make sense set a default then, to avoid getAction() complaining?

e.g.

  #[ODM\Field(name: 'action', type: Type::STRING)]
- protected ?string $action = null;
+ protected string $action = self::ACTION_CREATE;

(or whatever, if the syntax is slightly off)

Not sure if bug or feature but I can't have have only #[Loggable] + #[Versioned] in an entity but it forces me to use both #[Loggable] + #[Revisionable] + #[Versioned]

Class "App\Entity\VendorOrder" has "Gedmo\Mapping\Annotation\Versioned" annotated fields but is missing the "Gedmo\Mapping\Annotation\Revisionable" class annotation.

I'd need to see an entity to look at this more. The loggable mapping didn't change any and the revisionable mapping only looks for the Revisionable and Versioned attributes.

# services.yaml
    gedmo.mapping.driver.attribute:
        class: Gedmo\Mapping\Driver\AttributeReader

    gedmo.listener.revisionable:
        class: Gedmo\Revisionable\RevisionableListener
        tags:
            - { name: doctrine.event_listener, event: 'onFlush' }
            - { name: doctrine.event_listener, event: 'loadClassMetadata' }
            - { name: doctrine.event_listener, event: 'postPersist' }
        calls:
             - [ setAnnotationReader, [ '@gedmo.mapping.driver.attribute' ] ]

# doctrine.yaml
orm:
  # ...
  entity_managers:
    default:
      # ...
      mappings:
        # ...
        gedmo_loggable:
          type: attribute
          prefix: Gedmo\Loggable\Entity
          dir: "%kernel.project_dir%/vendor/gedmo/doctrine-extensions/src/Loggable/Entity/"
          alias: GedmoLoggable # (optional) it will default to the name set for the mappingmapping
          is_bundle: false
        revisionable:
          type: attribute
          alias: GedmoRevisionable
          prefix: Gedmo\Revisionable\Entity
          dir: "%kernel.project_dir%/vendor/gedmo/doctrine-extensions/src/Revisionable/Entity"
          is_bundle: false

# stof_doctrine_extensions.yaml
stof_doctrine_extensions:
    default_locale: en_US
    orm:
        default:
            timestampable: true
            blameable: true
            loggable: true
            sortable: true

<?php

declare(strict_types=1);

namespace App\Entity;

use App\Repository\VendorRepository;
use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Mapping as ORM;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Gedmo\Mapping\Annotation\Loggable;
use Gedmo\Mapping\Annotation\Versioned;

#[Loggable]
#[ORM\Table(name: 'vendors')]
#[ORM\Entity(repositoryClass: VendorRepository::class)]
class Vendor
{
    #[Column(name: 'vendor_id', type: Types::INTEGER)]
    #[Id]
    #[GeneratedValue(strategy: 'IDENTITY')]
    private ?int $vendorId = null;

    #[Versioned]
    #[Column(name: 'name', type: Types::STRING, length: 255, unique: true)]
    private string $name;

    public function getVendorId(): ?int
    {
        return $this->vendorId;
    }

    public function getName(): string
    {
        return $this->name;
    }

    public function setName(string $name): void
    {
        $this->name = $name;
    }
}

⚠️ doesn't work but errors with

Class "App\Entity\Vendor" has "Gedmo\Mapping\Annotation\Versioned" annotated fields but is missing the "Gedmo\Mapping\Annotation\Revisionable" class annotation.

If I add #[Revisionable] (also to all other entities having #[Loggable]) then (both of them) works:

<?php

declare(strict_types=1);

namespace App\Entity;

use App\Repository\VendorRepository;
use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Mapping as ORM;
use Doctrine\ORM\Mapping\Column;
use Doctrine\ORM\Mapping\GeneratedValue;
use Doctrine\ORM\Mapping\Id;
use Gedmo\Mapping\Annotation\Loggable;
use Gedmo\Mapping\Annotation\Revisionable;
use Gedmo\Mapping\Annotation\Versioned;

#[Loggable]
#[Revisionable]
#[ORM\Table(name: 'vendors')]
#[ORM\Entity(repositoryClass: VendorRepository::class)]
class Vendor
{
    #[Column(name: 'vendor_id', type: Types::INTEGER)]
    #[Id]
    #[GeneratedValue(strategy: 'IDENTITY')]
    private ?int $vendorId = null;

    #[Versioned]
    #[Column(name: 'name', type: Types::STRING, length: 255, unique: true)]
    private string $name;

    public function getVendorId(): ?int
    {
        return $this->vendorId;
    }

    public function getName(): string
    {
        return $this->name;
    }

    public function setName(string $name): void
    {
        $this->name = $name;
    }
}

I can try to set up a full reproducer next week if this isn't enough to figure it out

edit: not registering gedmo.listener.revisionable makes the old code work with this branch (but then the revisionable stuff won't obviously work)

The only thing I can think of without building a full reproducer is that the config being passed by reference is causing an unexpected side-effect, adding a dump($config) before this line would confirm that.

The only thing I can think of without building a full reproducer is that the config being passed by reference is causing an unexpected side-effect, adding a dump($config) before this line would confirm that.

Apparently, I can't use the same config key name (versioned) in both extensions since it looks like the ExtensionMetadataFactory::getExtensionMetadata() method doesn't pass a "clean" config array into the mapping drivers (no idea if I'd call this a bug or a feature at this point, but here we are).

I'll have to think about ways to improve this because the issue's really a symptom of a bigger design concern overall.

I've changed the config key, so that conflict shouldn't come up anymore.

I've changed the config key, so that conflict shouldn't come up anymore.

Unfortunately doesn't seem to help. Exactly same thing as in #2825 (comment)

Dumping the failing config:

Unfortunately doesn't seem to help. Exactly same thing as in #2825 (comment)

I get what the mapping drivers are trying to do here (warn if you use #[Versioned] on a property without the #[Loggable] or #[Revisionable] attributes), but the only way around that is to introduce an entirely new attribute (similar for XML and (deprecated) YAML)) just to not use the existing config names. And that is frustrating.

Do you know when this new extension will be released ?
Any help needed ?

I get what the mapping drivers are trying to do here (warn if you use #[Versioned] on a property without the #[Loggable] or #[Revisionable] attributes), but the only way around that is to introduce an entirely new attribute (similar for XML and (deprecated) YAML)) just to not use the existing config names. And that is frustrating.

I gave in, the revisionable extension will no longer use the #[Versioned] attribute but instead uses #[KeepRevisions] (and replace gedmo:versioned with gedmo:keep-revisions for the XML side of things). That should clear all the cross-extension config conflicts up.

Hello @mbabker,

I wanted to kindly ask if you could share some visibility on the status of this PR. Do you have an idea of when it might be merged, and are there specific blockers preventing it from moving forward?

We have several use cases that this PR directly addresses, and we are now reaching a stage where we’re considering whether to fork this PR to move ahead, rather than continue maintaining our own temporary solution with last doctrine updates. Before doing so, we would much prefer to align with the upstream direction.

Would it also be possible to create a release candidate including this PR, so that we can test it directly against our projects? That would help us validate our scenarios and potentially provide feedback that benefits the broader community.

Thank you again for your efforts and for any guidance you can provide.

I wanted to kindly ask if you could share some visibility on the status of this PR. Do you have an idea of when it might be merged, and are there specific blockers preventing it from moving forward?

I've gone as far with this as I can and it really just needs help testing from users. I don't have merge rights here so I can't personally do anything else with this right now other than keep it up-to-date, but in my mind since the bulk of it is a clone of the loggable extension with the adjustments in the data storage layer (switching from the DBAL's deprecated array type to JSON and switching from PHP serialized values to database normalized values through the MongoDB ODM and DBAL types APIs) it should be pretty much ready to go. The worst issues this PR's surfaced have been more design quirks with the library overall than bugs in the new extension, so to me that's also a good sign things should be ready to go.

Thank you for the clarifications — we will make an attempt to switch over to your PR. We really appreciate the effort you’ve put into maintaining this branch and this feature for more than a year.

Dear maintainers, would it be possible to consider merging this PR, perhaps under an experimental status as some frameworks do? This would allow the community to be aware of the feature’s current status, while still limiting the risks, which in this case appear to be quite minimal. At the same time, it would also allow us to move forward collectively on this topic.